Repository navigation
SimplePropertyMapping/CalculateMapping: skip inconvertible per-class unit specializations - #854
Open
ManasBagul23 wants to merge 1 commit into
Conversation
…unit specializations Both writeUnitValue() implementations already guard against calling toStandardUnit()/fromStandardUnit() on an InconvertibleUnitDatatype for the propertys own matched unit (the dbpedia#630 fix in SimplePropertyMapping, and the top-level check in CalculateMapping). But both also separately loop over context.ontology.specializations -- a per-class override of the property range, independent of the propertys own unit -- and call fromStandardUnit() on whatever specificPropertyUnit that lookup returns, unconditionally. Currency units (usDollar, euro, poundSterling, japaneseYen, russianRouble, unitedArabEmiratesDirham, ...) are registered as InconvertibleUnitDatatype in OntologyDatatypes.scala, so a class specializing a dimension property (e.g. a money amount) to one of these currencies hits the exact same always-throws fromStandardUnit()/UnsupportedOperationException("Tried to convert inconvertible unit") this time regardless of whether the property own matched unit was convertible. Skip writing that specific-class property when its specialized unit is inconvertible, consistent with how the property own unit is already handled, rather than crash or fabricate a value with no meaningful standard-unit basis. Related to dbpedia#582
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Description Check |  |
Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | |
The title clearly and concisely describes the main change: both mappings skip inconvertible per-class unit specializations. |
| Docstring Coverage | |
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0… |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Both
SimplePropertyMapping.writeUnitValueandCalculateMapping.writeUnitValuealready guard against callingtoStandardUnit()/fromStandardUnit()on anInconvertibleUnitDatatypefor the property's own matched unit — the#630fix inSimplePropertyMapping, and the top-levelunit.isInstanceOf[InconvertibleUnitDatatype]check inCalculateMapping(the latter addressed for the conversion itself in #853).But both functions also separately loop over
context.ontology.specializations— a per-class override of the property's range, looked up independently of the property's own unit — and callfromStandardUnit()on whateverspecificPropertyUnitthat lookup returns, unconditionally:Currency units (
usDollar,euro,poundSterling,japaneseYen,russianRouble,unitedArabEmiratesDirham, ...) are registered asInconvertibleUnitDatatypeinOntologyDatatypes.scala. So a class that specializes a dimension property (e.g. a money amount like budget/revenue) to one of these currencies hits the exact same always-throwsfromStandardUnit()→UnsupportedOperationException("Tried to convert inconvertible unit")— the same failure reported in #582 — regardless of whether the property's own matched unit was convertible.SimplePropertyMappingis the common, everyday property-mapping path (used for any infobox property with a unit/dimension range), so this is plausibly the dominant real-world trigger for that exception, more so than the narrowerCalculateMappingpath fixed in #853.Fix
Skip writing that specific-class property when its specialized unit is inconvertible, consistent with how the property's own unit is already handled — there's no meaningful "standard unit" basis to convert a currency amount from, so fabricating a value would be as wrong as crashing.
Related to #582 (same failure, different/more common trigger than #853).
Summary by CodeRabbit